Ticket #799 (closed defect: invalid)

Opened 3 years ago

Last modified 3 years ago

gensym is per-thread, seems like a bug to me.

Reported by: mt Owned by:
Priority: normal Milestone:
Component: other Version: trunk
Keywords: Cc:

Description

I just had an annoying bug that turned out to be due to gensyms being only unique for a given thread. I see that this was explicitly changed here:

http://trac.clozure.com/ccl/changeset/6016

But I maintain this is a bug (and different from other Common Lisp implementations).

Change History

comment:1 Changed 3 years ago by gb

  • Status changed from new to closed
  • Resolution set to invalid

Making *GENSYM-COUNTER* shared between all threads could make simple things like

? (setq *gensym-counter* 1234)
1234
? (gensym)
;;; Maybe something like #:G1234, maybe something totally unrelated if
;;; some other thread modified *GENSYM-COUNTER* after we did ...
;;; There are certainly other ways to lose here.  
;;; It's also true that this might behave as exected a high percentage
;;; of the time.  Whether occasional failures are catastrophic or merely
;;; annoying is application-dependent;  IIRC, in the case that motivated
;;; the change where the current behavior was introduced, the affected customer
;;; considered it to be somewhere between those extremes.

behave in ways that violate what seem to me to be reasonable assumptions. It's not clear that there's a good way to avoid this.

In at least one native-threaded implementation that doesn't bind *GENSYM-COUNTER* per thread, the case where GENSYM is called without an explicit suffix just does an INCF of *GENSYM-COUNTER* after reading its value. Two concurrent threads calling GENSYM at essentially the same time (and reading the same pre-incremented value of the counter) may produce results with STRING= pnames (which seems to violate your expectation that they be unique) and may cause *GENSYM-COUNTER* to be incremented by 1 instead of 2 (because of the unpredictable order in which read/modify/write operations occur.)

I can't tell from your message whether you expect all threads to share a global binding of *GENSYM-COUNTER* (as if that either was guaranteed to produce a series of visually unique gensyms or didn't have the other problems mentioned above) or whether the fact that *GENSYM-COUNTER* is bound to 0 in each thread is of greater concern. I care more that it's thread-local than I do what the initial value is, and if you have control over thread creation you can obviously set *GENSYM-COUNTER* to some arbitrary per-thread value and greatly reduce the chance that any two gensyms will have pnames that're STRING=.

If you're talking about fact that - in the unsolicited threads created by SLIME to handle selection evaluation, as if it was a good idea to evaluate each selection in a new thread and new dynamic context - *GENSYM-COUNTER* always cycles through the same set of values from 0 ... well, I'd say that this is a relatively benign example of why SLIME's behavior is misguided. You can change the default initial value of the per-thread binding to something large and random via:

(ccl::def-standard-initial-binding *gensym-counter* (- most-positive-fixnum 1000000))

and if you wanted to work harder to guarantee that each thread uses a unique range of values, I suppose that that might be possible.

If your concern is something that I haven't guessed, please reopen this ticket and articulate that concern. The current behavior is intentional and I believe that it's preferable to what you seem to be proposing. Arbitrary assignments to shared global variables generally can't be made thread-safe, and even if the violations of thread safety in this case would be relatively benign (unexpected changes to the value of *GENSYM-COUNTER* that're difficult to predict or reproduce), it's hard to see where this would be desirable.

comment:2 follow-up: ↓ 4 Changed 3 years ago by gz

It seems like a lock would address these problems, and given all that gensym does, the added overhead would not be a major concern. Then we wouldn't have to choose between having *gensym-counter* repeat occasionally and unpredictably, or predictably always, neither of which seems appealing to me.

comment:3 Changed 3 years ago by rme

Users have run across this before when trying to use gensym as a shortcut way to generate lisp-wide unique strings.

It's straightforward and probably better style (IMO) to implement some other way of providing lisp-wide unique strings if that's what is desired. I don't know what usage mt has in mind.

comment:4 in reply to: ↑ 2 ; follow-up: ↓ 5 Changed 3 years ago by gb

Replying to gz:

It seems like a lock would address these problems, and given all that gensym does, the added overhead would not be a major concern. Then we wouldn't have to choose between having *gensym-counter* repeat occasionally and unpredictably, or predictably always, neither of which seems appealing to me.

You mean that my example above should be written as:

 (with-lock-grabbed (*gensym-lock*)
   (setq *gensym-counter* 1234)
   (sleep 3600) ; or do other work for an arbitrary amount of time
   (gensym) ; predictably #:G1234
   )
;;; Presumably, a random assignment to *GENSYM-COUNTER* would only
;;; be legal if it's made with the lock held.

Or is there some other way in which a lock could be used ?

(I agree that there's no reason to be afraid of using a lock or some flavor of ATOMIC-INCF or something similar if doing so solved a problem.)

If *GENSYM-COUNTER* wasn't exposed as something that the user can modify in order to affect the next value returned by GENSYM, then sure: inside the implementation, you make sure that only one thread can read and post-increment the counter at a time. Unfortunately, the counter is exposed.

There seem to be at least two sets of expectations that users could have:

  1. A function can make a series of calls to GENSYM with *GENSYM-COUNTER* as an implicit suffix will yield a series of results with predictable names.
  1. GENSYM generally returns values that're globally unique.

I don't know of a way to satisfy both of these expectations. In some cases, you can achieve (1) by binding *GENSYM-COUNTER* while generating that sequence, and you may be able to get closer to (2) by randomizing the initial value of the per-thread bindings.

I don't know how much code assigns to *GENSYM-COUNTER*. If that's the big problem with having it be shared, I don't have too much of a problem with deprecating that (and recommending binding it instead of assigning to it), and if we ignore assignments to the shared variable then a lock/atomic operation would indeed solve a problem.

comment:5 in reply to: ↑ 4 Changed 3 years ago by gz

Replying to gb:

If *GENSYM-COUNTER* wasn't exposed as something that the user can modify in order to affect the next value returned by GENSYM, then sure: inside the implementation, you make sure that only one thread can read and post-increment the counter at a time. Unfortunately, the counter is exposed.

And since the counter is exposed, it's up to the user who messes with *gensym-counter* in a multi-threaded application to serialize his use of gensym and his uses of *gensym-counter*. I guess some might find that surprising, but I don't.

There seem to be at least two sets of expectations that users could have:

  1. A function can make a series of calls to GENSYM with *GENSYM-COUNTER* as an implicit suffix will yield a series of results with predictable names.
  1. GENSYM generally returns values that're globally unique.

I don't know of a way to satisfy both of these expectations.

Depends on what you mean by predictable. Certainly with a global *gensym-counter*, a series of calls to gensym in your application will, globally, yield a series of results with predictable names. What it will not guarantee is that, looking at one thread in isolation and with no control over what other threads are doing, you will get a predictable sequence for calls within that thread. That seems like a very specific localized requirement, and if any user happened to have that requirement, they can just bind *gensym-counter* around the code and/or the thread that has those expectations.

I don't know how much code assigns to *GENSYM-COUNTER*. If that's the big problem with having it be shared, I don't have too much of a problem with deprecating that (and recommending binding it instead of assigning to it), and if we ignore assignments to the shared variable then a lock/atomic operation would indeed solve a problem.

Great.

comment:6 Changed 3 years ago by rme

  • Component changed from IDE to other

As of r14553, *gensym-counter* is global. Here is the commit message:

Make *GENSYM-COUNTER* global. This means (GENSYM) will generate globally unique pnames, though it also means multi-threaded code that explicitly modifies *GENSYM-COUNTER* may need to do appropriate locking or binding to achieve whatever effect it is hoping to achieve.

Note: See TracTickets for help on using tickets.